-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PMM-11261 Update PMM via watchtower #2844
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3 #2844 +/- ##
=====================================
Coverage ? 43.31%
=====================================
Files ? 365
Lines ? 42707
Branches ? 0
=====================================
Hits ? 18498
Misses ? 22601
Partials ? 1608
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
managed/cmd/pmm-managed/main.go
Outdated
@@ -969,6 +969,20 @@ func main() { //nolint:cyclop,maintidx | |||
|
|||
dumpService := dump.New(db) | |||
|
|||
// Get the hostname from the environment variable | |||
watchtowerHostname := os.Getenv("PMM_WATCHTOWER_HOSTNAME") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a different envvar name at https://github.com/percona/pmm/pull/2844/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R26. Just to double-check there is no typo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep in mind that, host
would contain the port, while hostname
wouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
managed/cmd/pmm-managed/main.go
Outdated
if watchtowerHostname == "" { | ||
watchtowerHostname = "http://watchtower:8080" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if watchtowerHostname == "" { | |
watchtowerHostname = "http://watchtower:8080" | |
} | |
if watchtowerHost == "" { | |
watchtowerHost = "http://watchtower:8080" | |
} |
Co-authored-by: Alex Demidoff <[email protected]>
Co-authored-by: Alex Demidoff <[email protected]>
managed/services/server/updater.go
Outdated
case err != nil && !os.IsNotExist(err): | ||
s.l.WithError(err).Error("Failed to read file") | ||
return nil, errors.Wrap(err, "failed to read file") | ||
case os.Getenv("PMM_SERVER_UPDATE_VERSION") != "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this shouldn't be an if
statement, outside of the switch
? It does not seem to depend on the result of os.ReadFile
, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not about depending on the result of readfile, but more about priorities. I can rewrite it to ifs
instead of switch-case
Co-authored-by: Alex Demidoff <[email protected]>
Co-authored-by: Alex Demidoff <[email protected]>
Co-authored-by: Alex Demidoff <[email protected]>
PMM-11261
Link to the Feature Build: SUBMODULES-3562
If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:
If this PR is related to some other PRs in this or other repositories, please provide links to those PRs: